Add nullability to more MVC assemblies #31338

pranavkm · 2021-03-28T19:06:07Z

JamesNK · 2021-03-28T21:31:44Z

src/Mvc/Mvc.DataAnnotations/src/MvcDataAnnotationsLocalizationOptions.cs

        /// The delegate to invoke for creating <see cref="IStringLocalizer"/>.
        /// </summary>
-        public Func<Type, IStringLocalizerFactory, IStringLocalizer> DataAnnotationLocalizerProvider;
+        public Func<Type, IStringLocalizerFactory, IStringLocalizer> DataAnnotationLocalizerProvider = null!;


Huh, weird that this is a field

Oh wow, did not notice that. API-review fail :)

src/Mvc/Mvc.NewtonsoftJson/src/BsonTempDataSerializer.cs

JamesNK · 2021-03-28T21:37:25Z

src/Mvc/Mvc.ViewFeatures/src/Controller.cs

                }

-                return _viewData;
+                return _viewData!;


I guess. Although the if block has a nice comment which says it's only ever run inside a unit test. I'd prefer to keep it that way.

src/Mvc/Mvc.ViewFeatures/src/Rendering/ViewContext.cs

src/Mvc/Mvc.DataAnnotations/src/DataAnnotationsModelValidator.cs

BrennanConroy · 2021-03-29T16:44:45Z

src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs

            }

-            if (!(exception is JsonException || exception is OverflowException || exception is FormatException))
+            if (exception is not null && exception is not JsonException or OverflowException or FormatException)


BrennanConroy · 2021-03-29T16:52:40Z

src/Mvc/Mvc.ViewFeatures/src/AttributeDictionary.cs

                }

-                public string Current => _attributes.Get(_index).Key;
+                public string? Current => _attributes.Get(_index).Value;


Straight up bug. It's an enumerator for values (which can be null) but was returning the keys (which are non-nullable) instead.

Nothing was affected by this though?

Apparently not. You'd think we have a test for this. The usually pattern is to iterate over the dictionary itself which has the correct semantics.

src/Mvc/Mvc.ViewFeatures/src/TempDataDictionary.cs

src/Mvc/Mvc.Razor/src/RazorView.cs

* [x] MVC.Cors * [x] Mvc.DataAnnotations * [x] Mvc.Localization * [x] Mvc.Newtonsoft.Json * [ ] Mvc.ViewFeatures (partial)

pranavkm · 2021-03-31T23:21:13Z

This has been updated. I also covered nullability for Mvc.Razor while addressing some feedback

BrennanConroy

Some comments

src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs

src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonPatchInputFormatter.cs

src/Mvc/Mvc.Razor/src/MvcRazorLoggerExtensions.cs

BrennanConroy · 2021-04-01T17:20:35Z

src/Mvc/Mvc.Razor/src/RazorPage.cs

        /// An <see cref="HttpContext"/> representing the current request execution.
        /// </summary>
-        public HttpContext Context => ViewContext?.HttpContext;
+        public HttpContext Context => ViewContext?.HttpContext!;


ViewContext can't be null

In practice no. But in theory, if you're unit testing, you could have chosen to not initialize it. We want to avoid producing a null ref from the framework code in that case.

var page = new RazorPage(); page.HttpContext // fine if it's null here, but we want to avoid a NullReferenceException

src/Mvc/Mvc.Razor/src/RazorPageActivator.cs

src/Mvc/Mvc.Razor/src/TagHelpers/UrlResolutionTagHelper.cs

BrennanConroy · 2021-04-01T17:30:23Z

src/Mvc/Mvc.ViewFeatures/src/AttributeDictionary.cs

                }

-                public string Current => _attributes.Get(_index).Key;
+                public string? Current => _attributes.Get(_index).Value;


Nothing was affected by this though?

BrennanConroy · 2021-04-01T17:33:04Z

src/Mvc/Mvc.ViewFeatures/src/TempDataDictionary.cs


+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        [MemberNotNull(nameof(_initialKeys), nameof(_retainedKeys), nameof(_data))]
+        private void AssertLoaded()


Make this method debug only?

BrennanConroy · 2021-04-01T17:33:50Z

src/Mvc/Mvc.ViewFeatures/src/TempDataDictionary.cs

            public void Reset()
            {
-                _enumerator.Reset();
+                ((IEnumerator < KeyValuePair<string, object?>>)_enumerator).Reset();


Haha, didn't get fixed

BrennanConroy · 2021-04-01T17:36:56Z

src/Mvc/Mvc.ViewFeatures/src/ViewEngines/ViewEngineResult.cs

        /// </exception>
        /// <returns>This <see cref="ViewEngineResult"/> if <see cref="Success"/> is <c>true</c>.</returns>
-        public ViewEngineResult EnsureSuccessful(IEnumerable<string> originalLocations)
+        [MemberNotNull(nameof(View))]


This doesn't make sense

src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs

src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonPatchInputFormatter.cs

src/Mvc/Mvc.Razor/src/MvcRazorLoggerExtensions.cs

pranavkm · 2021-04-01T17:43:13Z

src/Mvc/Mvc.Razor/src/RazorPage.cs

        /// An <see cref="HttpContext"/> representing the current request execution.
        /// </summary>
-        public HttpContext Context => ViewContext?.HttpContext;
+        public HttpContext Context => ViewContext?.HttpContext!;


In practice no. But in theory, if you're unit testing, you could have chosen to not initialize it. We want to avoid producing a null ref from the framework code in that case.

var page = new RazorPage(); page.HttpContext // fine if it's null here, but we want to avoid a NullReferenceException

src/Mvc/Mvc.Razor/src/RazorPageActivator.cs

pranavkm · 2021-04-01T17:46:07Z

src/Mvc/Mvc.Razor/src/RazorPageBase.cs

        /// Gets the dynamic view data dictionary.
        /// </summary>
-        public dynamic ViewBag => ViewContext?.ViewBag;
+        public dynamic ViewBag => ViewContext?.ViewBag!;


Same weird deal as RazorPageBase

pranavkm · 2021-04-01T17:46:45Z

src/Mvc/Mvc.Razor/src/TagHelpers/UrlResolutionTagHelper.cs

                {
-                    string resolvedUrl;
-                    if (TryResolveUrl(stringValue, resolvedUrl: out resolvedUrl))
+                    if (TryResolveUrl(stringValue, resolvedUrl: out string? resolvedUrl))


It's an overloaded method and you have to specify the type for resolution

pranavkm · 2021-04-01T17:47:29Z

src/Mvc/Mvc.ViewFeatures/src/AttributeDictionary.cs

                }

-                public string Current => _attributes.Get(_index).Key;
+                public string? Current => _attributes.Get(_index).Value;


Apparently not. You'd think we have a test for this. The usually pattern is to iterate over the dictionary itself which has the correct semantics.

ghost · 2021-04-01T18:24:49Z

Hello @pranavkm!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

pranavkm requested a review from javiercn as a code owner March 28, 2021 19:06

pranavkm requested review from BrennanConroy and JamesNK March 28, 2021 19:06

pranavkm added this to the 6.0-preview4 milestone Mar 28, 2021

pranavkm added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Mar 28, 2021

JamesNK reviewed Mar 28, 2021

View reviewed changes

mkArtakMSFT linked an issue Mar 29, 2021 that may be closed by this pull request

Update ASP.NET Core to use C# 8's nullable reference types #5680

Closed

BrennanConroy reviewed Mar 29, 2021

View reviewed changes

Add nullability to more MVC assemblies

5500c69

* [x] MVC.Cors * [x] Mvc.DataAnnotations * [x] Mvc.Localization * [x] Mvc.Newtonsoft.Json * [ ] Mvc.ViewFeatures (partial)

pranavkm force-pushed the prkrishn/mvc-nullable branch from 20f1478 to b1ff989 Compare March 31, 2021 17:31

Fixup

76b7f9d

pranavkm force-pushed the prkrishn/mvc-nullable branch from b1ff989 to 76b7f9d Compare March 31, 2021 20:01

Update src/Mvc/Mvc.ViewFeatures/src/Rendering/ViewContext.cs

6b45ab4

BrennanConroy approved these changes Apr 1, 2021

View reviewed changes

pranavkm commented Apr 1, 2021

View reviewed changes

pranavkm added 2 commits April 1, 2021 11:05

Apply suggestions from code review

8d32351

Update RazorPagePropertyActivator.cs

f66ff90

pranavkm added the auto-merge label Apr 1, 2021

ghost merged commit b12b64e into main Apr 1, 2021

ghost deleted the prkrishn/mvc-nullable branch April 1, 2021 19:47

This pull request was closed.

Add nullability to more MVC assemblies #31338

Add nullability to more MVC assemblies #31338

Uh oh!

Conversation

pranavkm commented Mar 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pranavkm commented Mar 31, 2021

Uh oh!

BrennanConroy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Apr 1, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pranavkm commented Mar 28, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.